Repository navigation
Adapt the PR review skill to any language and infrastructure - #4
Merged
Merged
Conversation
Feedback was that ardan-pr-review was too focused on Go. It now reviews Rust, Python, Terraform/OpenTofu, Helm, Kustomize/K8s, Ansible, Dockerfiles, and CI pipelines (GitHub Actions, GitLab, Jenkins, ...) as first-class code, with each ecosystem's own static analysis. - intake: path- and content-aware classification (charts, kustomize, K8s manifests, Ansible, CI systems); infrastructure counts as prod so correctness/tests agents run on infra-only PRs; symbols for Rust, Python, Terraform addresses, and Helm templates; per-language signals; stack.tsv (units per ecosystem), idioms=, unmapped.txt; Go layers only on Go files; --include-generated opt-in (sh and PowerShell) - checks: cargo fmt/clippy/test/audit/deny (--locked); terraform or tofu fmt + init -backend=false + validate (temp TF_DATA_DIR, read-only lock) and tflint; helm lint/template and kustomize build into kubeconform + kube-linter; trivy/checkov IaC scan; ruff, mypy, pytest, bandit, pip-audit without caches; ansible-lint, yamllint, hadolint, actionlint, zizmor; user-approved repo gates (--extra); a lens column in summary.tsv; go build -o /dev/null so a main package never writes its binary into the repo - discovery agent: reads configs only, finds ecosystems the scripts miss and the repo's own quality gates, offered to the user as extra checks; extract-findings --discovery saves its reply - lenses, contract, and roles are language-neutral; per-language idioms live in references/langs/*.md and agents load only the ones in scope - SKILL.md: discovery step, repo-gate approval, generated files skip / secret-scan / review choice, test setup for Rust, Python, Terraform Tests: zarf/tests/ardan-pr-review builds a fixture repo with real problems in every ecosystem and asserts sh/PowerShell parity, golden intake and check results, and a read-only working tree, with stub analyzers on the host (BSD and GNU) and the real tools at pinned versions in a Docker image. `make test` runs lint and both modes; CI runs them on PRs touching the skill.
Several checks executed code the reviewed change controls, and --no-tests didn't stop them: cargo clippy/check (build scripts, proc macros), terraform/tofu init + validate (PR-chosen providers and modules), tflint --init (repo plugins), mypy and ansible-lint (repo plugins and rules), pip-audit dependency resolution (setup.py), repo gates, and the existing go test / cargo test / pytest runs. - intake: trust=maintainer only for --pr when GitHub reports the PR author has write/maintain/admin access and the head branch is in the base repo (commit authors are not used: they can be forged); every other scope or gh error is trust=untrusted, recorded with a reason (sh and PowerShell) - checks: read trust from scope.txt; when untrusted, skip tests, cargo clippy/check/test, terraform validate, ansible-lint, golangci-lint with custom linter plugins, and repo gates; run tflint and mypy with neutral configs; pip-audit always audits pins as written (--no-deps --disable-pip) - SKILL.md, contract, lenses, and idioms: no tests, builds, or PoCs of the change unless trust=maintainer; skipped rows become one Open check - tests: a fake gh drives --pr runs for maintainer, read-only author, and fork; golden results for untrusted and trusted check runs
The custom-linter guard grepped .golangci.* for "custom:", which other spellings of the same config (JSON, TOML tables, YAML flow style or anchors) slip past. For an untrusted change golangci-lint now runs with --no-config, so the repo's config is never parsed; a maintainer PR keeps the repo's config. The new test exposed two older bugs that kept golangci-lint from running at all: - sh: config detection used `ls a b c`, which fails when any one file is missing (intake and checks; also semgrep in intake) - PowerShell: Get-ChildItem hides dotfiles on Unix without -Force Tests: the fixture ships a .golangci.yml naming a custom plugin; the suite asserts --no-config for untrusted runs and the repo config for the maintainer run (both ports); golangci-lint pinned in the tools image.
CI's shellcheck flags SC2015 on these (the local 0.11.0 doesn't): an &&/|| chain reads like if/else but runs the || branch when B fails too.
CI (amd64) failed tf-validate on a module whose lock file was made on arm64: lock files record h1 hashes only for the platforms someone ran init on, a read-only init can't add the current one, and validate then rejects the cached provider. Reviews from another OS or CPU than the author's would always hit this. init now runs normally (locked versions still apply; nothing upgrades without -upgrade), and the original .terraform.lock.hcl is restored byte for byte after validate (sh and PowerShell); a lock file init created is removed as before. Tests: the OpenTofu pass runs as a maintainer PR, so init/validate actually run and the suite checks that tofu's lock rewrite is undone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Feedback:
ardan-pr-reviewwas too focused on Go. Infrastructure files were classified as plain config, so an infra-only PR skipped the correctness and tests reviewers, and every check and checklist assumed Go.What
init -backend=false+ validate, plus tflint; Helm/Kustomize renders into kubeconform and kube-linter; trivy or checkov for IaC; ruff, mypy, pytest, bandit, pip-audit; ansible-lint, yamllint, hadolint, actionlint, zizmor. All read-only: Terraform uses a temp data dir and a read-only lock, Python runs without caches, andgo buildwrites to/dev/null(previously a singlemainpackage dropped its binary into the reviewed repo).cargo clippy/check/test, Terraform init/validate, linter plugins (tflint, mypy, ansible-lint, golangci-lint custom linters), repo gates, and security PoCs run only for--prwhen GitHub reports that the author has write access and the head branch is in the base repo (commit authors are not used — they can be forged). Everything else gets the read-only checks, with neutral configs where a repo config could load code; pip-audit never resolves dependencies.references/langs/*.md, and agents load only the ones the change needs.--include-generated, with the new size shown first).Tests
make test= lint +zarf/tests/ardan-pr-reviewin two modes:zarf/tests/ardan-pr-review/Dockerfile).A fixture repo with real problems in every ecosystem (plus a generated file and a symlink) is used to assert sh ↔ PowerShell parity, golden intake and check results, and a working tree left untouched by the checks (including the OpenTofu pass). CI runs lint, host mode on Linux and macOS, and container mode on PRs touching the skill.
Verified locally:
make testpasses (macOS host + arm64 container); actionlint and zizmor are clean on the new workflow. All pinned amd64 download URLs exist, but this PR's CI run is the first amd64 image build.Not yet exercised
The discovery agent and the rewritten prompts haven't been through a live end-to-end review; this PR tests the scripts.